Skip to content

Conversation

@matt-codecov
Copy link
Contributor

@matt-codecov matt-codecov commented Apr 4, 2025

this PR does three things:

  • creates a new unified Dockerfile.requirements
  • adds Makefile targets to build images with said Dockerfile.requirements
  • updates CI to use the new Makefile targets rather than cd-ing to apps/worker etc first

related:

background

as part of cutover, we want to make worker/api use the copy of shared from inside the repository rather than a specific hardcoded SHA that it fetches from GitHub. conceptually it's as simple as replacing shared = { sha = "abcdef1234567890" } with shared = { path = "../../libs/shared" } in pyproject.toml, but there are a few complications:

  • docker build can only access things in its working directory. no ..s. so we must run it from the repository root, not apps/worker or apps/codecov-api which is how we were building images before
  • shared is still a dependency that worker/api need to install, but changes to it are no longer reflected in pyproject.toml or uv.lock since we're not hardcoding a specific SHA anymore. changing shared thus won't change the requirements image tag or cache keys like we need. so we need a way to reflect changes to shared in those things

to solve these problems, i changed how umbrella builds containers and runs CI in three ways:

unified Dockerfile.requirements

as explained above, we need to build the requirements images from umbrella rather than worker/api's subdirectories. the worker/api Dockerfile.requirements files are almost identical, so i basically merged them to create a unified Dockerfile.requirements.

although the actual Dockerfile.requirements is shared, worker/api still have separate requirements images. the unified dockerfile takes an APP_DIR build argument (apps/worker or apps/codecov-api) and will use the pyproject.toml / uv.lock from inside that directory to build the image. if/when we move to a repo-wide python project, we can pass . in instead to move to a shared requirements image.

one difference between the unified requirements image and the versions in apps/worker and apps/codecov-api is that this one bind-mounts libs/shared inside the container. currently this doesn't matter, but after cutover this is what will allow shared = { path = "../../libs/shared" } to work.

Makefile targets

this PR adds worker.*, api.*, and shared.* targets to umbrella's Makefile. by default, any worker.* target will set some variables to worker/umbrella-appropriate values and then shell out to worker's Makefile, but we override the targets used to build requirements images so we can use the umbrella version explained above.

it also changes how we calculate REQUIREMENTS_TAG in two ways:

  • it uses a hash of umbrella's Dockerfile.requirements instead of the worker/api-specific ones
  • it adds a hash of all of libs/shared. any commit that touches libs/shared will trigger a rebuild that installs shared with those changes.
    • it does this with git archive --format=tar HEAD libs/shared | sha1sum | head -c 40 which is actually very fast
    • if we move away from installing shared as an actual package, we can just get rid of this logic entirely

(after cutover, we can probably eliminate the hard-to-read variable override functions and pattern rules)

CI change

this PR moves from the old working_directory argument i added to a new make_target_prefix argument i added. if a workflow is going to call make build, umbrella can use this argument to turn that into make worker.build which will run the special Makefile targets explained above

unfortunately, i couldn't totally get rid of working_directory. because of the way things work, things like junit.xml or app.tar are still dumped in apps/worker etc. so i renamed the argument output_directory

this PR also uses the new reqs_cache_key argument to supply a cache key for the requirements image that properly accounts for changes to libs/shared. the way this is done is messy, but it can be cleaned up after cutover when we can just replace the old way of computing the cache key.

@matt-codecov matt-codecov force-pushed the matt/umbrella-makefile branch from 74d1ede to d17ca70 Compare April 4, 2025 23:32
@codecov
Copy link

codecov bot commented Apr 4, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.50%. Comparing base (dd90d42) to head (c3b1ad7).
Report is 2 commits behind head on main.

✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #30   +/-   ##
=======================================
  Coverage   96.49%   96.50%           
=======================================
  Files        1654     1644   -10     
  Lines       93104    93023   -81     
  Branches     1464     1464           
=======================================
- Hits        89843    89771   -72     
+ Misses       2956     2947    -9     
  Partials      305      305           
Flag Coverage Δ
apiunit 98.02% <ø> (+0.01%) ⬆️
shared-docker-uploader 88.04% <ø> (-0.01%) ⬇️
workerintegration 42.83% <ø> (-0.03%) ⬇️
workerunit 90.50% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@codecov-notifications
Copy link

codecov-notifications bot commented Apr 4, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

✅ All tests successful. No failed tests found.

📢 Thoughts on this report? Let us know!

@codspeed-hq
Copy link

codspeed-hq bot commented Apr 4, 2025

CodSpeed Performance Report

Merging #30 will not alter performance

Comparing matt/umbrella-makefile (c3b1ad7) with main (dd90d42)

Summary

✅ 9 untouched benchmarks

@matt-codecov matt-codecov force-pushed the matt/umbrella-makefile branch from d17ca70 to 47cf38d Compare April 4, 2025 23:55
@matt-codecov matt-codecov changed the title Matt/umbrella makefile use unified requirements image which is local-shared-friendly Apr 8, 2025
@matt-codecov matt-codecov requested a review from a team April 8, 2025 00:09
@matt-codecov matt-codecov marked this pull request as ready for review April 8, 2025 00:09
@matt-codecov matt-codecov force-pushed the matt/umbrella-makefile branch from 47cf38d to c3b1ad7 Compare April 8, 2025 22:12
@matt-codecov matt-codecov enabled auto-merge April 8, 2025 22:12
@matt-codecov matt-codecov added this pull request to the merge queue Apr 8, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 8, 2025
@matt-codecov matt-codecov added this pull request to the merge queue Apr 9, 2025
Merged via the queue into main with commit 6e96c69 Apr 9, 2025
43 of 47 checks passed
@matt-codecov matt-codecov deleted the matt/umbrella-makefile branch April 9, 2025 00:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants